-
Notifications
You must be signed in to change notification settings - Fork 5.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: support atomic batch transactions #30271
base: main
Are you sure you want to change the base?
Conversation
@metamaskbot update-policies |
Updated and removed dependencies detected. Learn more about Socket for GitHub ↗︎
|
Policies updated. 🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff |
f377c94
to
29af3c0
Compare
@metamaskbot update-policies |
Policies updated. 🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff |
@metamaskbot update-policies |
Policies updated. 🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff |
Builds ready [d9364b5]
Page Load Metrics (1708 ± 101 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
1 similar comment
Builds ready [d9364b5]
Page Load Metrics (1708 ± 101 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
Builds ready [11fd386]
Page Load Metrics (1799 ± 71 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
Builds ready [0e2b89e]
Page Load Metrics (1649 ± 50 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
@metamaskbot update-policies |
Policies updated. 🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff |
@metamaskbot update-policies |
Policies updated. 🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff |
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Builds ready [41eade6]
Page Load Metrics (1863 ± 101 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [ad4e096]
Page Load Metrics (2073 ± 444 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
transactionController: TransactionController, | ||
networkController: NetworkController, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might it make more sense / match our existing patterns better to pass in hooks to the actual methods you're calling rather than the full controllers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, it definitely would.
In a future PR, we could even just pass in the messenger and invoke the controllers using the messenger actions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
throw rpcErrors.invalidInput( | ||
`Chain ID must match the dApp selected network: Got ${requestChainId}, expected ${dappChainId}`, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmmm I'm not sure if this is the right rpc error... Though I'm looking at the others and this may be our best match 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is our standard bad input error we use for all the transaction and signature calls from eth-json-rpc-middleware
.
Though the EIP-5792 specification is still being finalised and I believe they're looking at formalising error codes for different scenarios so will likely extend this in future.
} | ||
|
||
export function getTransactionReceiptsByBatchId( | ||
controller: TransactionController, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here: could we just pass a method in rather than the whole controller?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
} | ||
|
||
export async function getCapabilities( | ||
controller: TransactionController, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And same here: could we pass in a method rather than the whole controller?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Builds ready [28dccab]
Page Load Metrics (1882 ± 128 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Support batch transaction type.
eb51b9c
to
e38a988
Compare
Builds ready [e38a988]
Page Load Metrics (2004 ± 125 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Description
Support atomic batch transactions via EIP-5792 and EIP-7702.
Specifically:
@metamask/transaction-controller
to support adding batch transactions.@metamask/eth-json-rpc-middleware
to validate and process EIP-5792 RPC requests.@etheremjs/tx
to fix nonce type in EIP-7702 authorizations.TransactionType.batch
in confirmations.Related issues
Fixes #4209
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist